Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use windows-rs instead of winapi-rs #1938

Closed
wants to merge 1 commit into from
Closed

Use windows-rs instead of winapi-rs #1938

wants to merge 1 commit into from

Conversation

clemenswasser
Copy link
Contributor

We should probably adopt windows-rs since they are now the official bindings to Windows for Rust coming directly from Microsoft.

This PR is currently blocked by microsoft/windows-rs#81
This functionality is required in src/platform_impl/windows/drop_handler.rs.
Because of that this PR does not build and I can not test the changes I made.
Therefore I probably broke many things.

  • Tested on all platforms changed
  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • cargo doc builds successfully
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@maroider
Copy link
Member

I'm very sceptical of adopting windows-rs before the whole "prebuilt-bindings as a crate" thing (microsoft/windows-rs#432) is figured out, as build times seem to be impacted considerably, which is already something people are dissatisfied with Winit for. Cross-compiling from Linux to Windows is also broken in windows-rs (microsoft/windows-rs#638, which is blocked on rust-lang/rust#58713), and I'm fairly certain that some users of Winit cross-compile from Linux to Windows.

@clemenswasser
Copy link
Contributor Author

I'm very sceptical of adopting windows-rs before the whole "prebuilt-bindings as a crate" thing (microsoft/windows-rs#432) is figured out, as build times seem to be impacted considerably, which is already something people are dissatisfied with Winit for. Cross-compiling from Linux to Windows is also broken in windows-rs (microsoft/windows-rs#638, which is blocked on rust-lang/rust#58713), and I'm fairly certain that some users of Winit cross-compile from Linux to Windows.

Agree with all of your points. I wanted to make a draft PR sooner than later to discuss theses kinds of problems.
I would also like to wait a bit more to be a bit more flexible with regards to windows-rs development.

@madsmtm
Copy link
Member

madsmtm commented May 26, 2021

Another point, the direct dependency parking_lot uses winapi, so we would end up with both winapi and windows-rs in our dependency tree

@MarijnS95
Copy link
Member

@clemenswasser Why did this get closed, especially now that the prebuilt API crate has just been released? GNU cross-compilation support is being worked on next, and while it might definitely be a good idea to let it all stabilize for a little while and evaluate major pain points with the new windows-rs crate, now seems a better time than ever to be on top of this one.

@clemenswasser
Copy link
Contributor Author

@clemenswasser Why did this get closed, especially now that the prebuilt API crate has just been released? GNU cross-compilation support is being worked on next, and while it might definitely be a good idea to let it all stabilize for a little while and evaluate major pain points with the new windows-rs crate, now seems a better time than ever to be on top of this one.

Yes. I know. I am working on it. I closed this, because it is quite outdated and it's easier to start fresh.

@MarijnS95
Copy link
Member

@clemenswasser Glad to hear that, good to know and thank you for still working on this! Otherwise I might have started doing the same in parallel.

@maroider
Copy link
Member

maroider commented Nov 9, 2021

Since no-one else linked the new PR, then I guess I'll do so: #2057

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants